Assignment9+10: Parker Brown+ Leo Marlow #83
Conversation
Implemented RunAsnyc, runLongRunningAsync
…rown34/EWU-CSCD371-2024-Fall into Assignment9+10-Multithreading
Pbrown34
force-pushed
the
Assignment9+10-Multithreading
branch
from
59d50fa to
7c0ab22
Compare
There was a problem hiding this comment.
Instructions
- Implement
PingProcess'public Task<PingResult> RunTaskAsync(string hostNameOrAddress)✔- First implement
public void RunTaskAsync_Success()test method to testPingProcess.RunTaskAsync()using"localhost". ✔ - Do NOT use async/await in this implementation. ✔
- First implement
- Implement
PingProcess'async public Task<PingResult> RunAsync(string hostNameOrAddress)✔- First implement the
public void RunAsync_UsingTaskReturn_Success()test method to testPingProcess.RunAsync()using"localhost"without using async/await. ✔ - Also implement the
async public Task RunAsync_UsingTpl_Success()test method to testPingProcess.RunAsync()using"localhost"but this time DO using async/await. ✔
- First implement the
- Add support for an optional cancellation token to the
PingProcess.RunAsync()signature. ✔
Inside thePingProcess.RunAsync()invoke the token'sThrowIfCancellationRequested()method so an exception is thrown. ✔
Test that, when cancelled from the test method, the exception thrown is anAggregateException✔ with aTaskCanceledExceptioninner exception ✔ using the test methodsRunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrapping✔andRunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrappingTaskCanceledException✔ respectively. - Complete/fix AND test
async public Task<PingResult> RunAsync(IEnumerable<string> hostNameOrAddresses, CancellationToken cancellationToken = default)which executes ping for an array of hostNameOrAddresses (which can all be "localhost") in parallel, adding synchronization if needed. ✔
NOTE:- The order of the items in the stdOutput is irrelevant and expected to be intermingled.
- StdOutput must have all the ping output returned (no lines can be missing) even though intermingled. ❌
- Implement AND test
public Task<int> RunLongRunningAsync(ProcessStartInfo startInfo, Action<string?>? progressOutput, Action<string?>? progressError, CancellationToken token)usingTask.Factory.StartNew()and invokingRunProcessInternalwith aTaskCreationvalue ofTaskCreationOptions.LongRunningand aTaskSchedulervalue ofTaskScheduler.Current. Returning aTask<PingResult>is also okay. ✔
NOTE: This method does NOT useTask.Run. ✔
Extra Credit
- Test and implement
PingProcess.RunAsync(System.IProgress<T> progess)so that you can capture the output as it occurs rather than capturing the output only when the process finishes. ❌✔
Fundamentals
- Place all shared project properties into a
Directory.Build.Propsfile. - Place all shared project items into a
Directory.Build.targetsfile. - Ensure nullable reference types is enabled ✔
- Ensure that you turn on code analysis for all projects(EnableNETAnalyzers) ✔
- Set
LangVersionand theTargetFrameworkto the latest released versions available (preview versions optional) ✔ - and enabled .NET analyzers for both projects ✔
- For this assignment, consider using
Assert.AreEqual<T>()(the generic version) ✔ - All of the above should be unit tested ✔
- Choose simplicity over complexity ✔
Overall
You guys did a good job! What I would suggest working on is ensuring that you aren't using Task.Run since it explicitly states not to. Then I would try to get the standard output working. I'll update my PR and change the ❌'s to ✔'s when you fix it :)
| } | ||
| return new PingResult(process.ExitCode, stdOutput); | ||
| } | ||
| public Task<PingResult> RunTaskAsync(string hostNameOrAddress, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
You shouldn't need the second parameter CancellationToken cancellationToken = default
Assignment/PingProcess.cs
Outdated
|
|
||
| Process process = RunProcessInternal(startInfo, updateStdOutput, progressError, token); | ||
|
|
||
| await Task.Run(() => process.WaitForExit(), token); |
There was a problem hiding this comment.
The instructions say to use Task.Factory.StartNew() and use TaskCreationOptions.LongRunning, so just make sure to replace Task.Run with Task.Factory.StartNew(). You might have to refactor this method slightly
Assignment.tests/PingProcessTests.cs
Outdated
| } | ||
|
|
||
| [TestMethod] | ||
| async public Task RunAsync_UsingTpl_Success() |
There was a problem hiding this comment.
Nit: White space, just move the test right one tab
Assignment.tests/PingProcessTests.cs
Outdated
| Assert.AreEqual(numbers.Count() + 1, lineCount); | ||
| } | ||
|
|
||
| // private readonly string PingOutputLikeExpression = @" |
There was a problem hiding this comment.
It seems like you completely removed the WildCardPattern class, I think if you add it back you shouldn't have to comment out this method, and the following methods, but either way I don't blame you because of how many issues it caused me and my partner. But you will need this to verify "StdOutput must have all the ping output returned (no lines can be missing) even though intermingled." If we get it working I'll let you know.
There was a problem hiding this comment.
Hey, we got it working, this is how we formatted it:
private readonly string PingOutputLikeExpression = @"
PING * 56 data bytes
64 bytes from * (::1): icmp_seq=* ttl=* time=* ms
64 bytes from * (::1): icmp_seq=* ttl=* time=* ms
64 bytes from * (::1): icmp_seq=* ttl=* time=* ms
64 bytes from * (::1): icmp_seq=* ttl=* time=* ms
--- * ping statistics ---
* packets transmitted, * received, *% packet loss, time *ms
rtt min/avg/max/mdev = */*/*/* ms
".Trim();I don't think there's any other way to format it than this. Just be sure to add the WildCardPattern class back and then you should be able to use AssertValidPingOutput in the tests.
| { | ||
| if (ex.InnerException is TaskCanceledException) | ||
| { | ||
| throw new AggregateException(ex.InnerException); |
There was a problem hiding this comment.
You shouldn't need to throw a new AggregateException(ex.InnerException) here when you're already expecting the exception.
There was a problem hiding this comment.
This is a wrapper for aggregate expression. The expected was a taskcanceledExemption which then is rewrapped
Assignment.tests/PingProcessTests.cs
Outdated
| async public Task RunAsync_MultipleHostAddresses_True() | ||
| { | ||
| string[] hostNames = new string[] { "-c 4 localhost", "-c 4 localhost", "-c 4 localhost", "-c 4 localhost" }; | ||
| foreach (var hostName in hostNames) |
There was a problem hiding this comment.
I believe that since this is ran inside of the foreach loop, it negates the parallel execution since it processes each hostname one by one. I would suggest just getting rid of the foreach loop.
| PingResult result = Sut.Run("-c 4 localhost"); | ||
| Assert.AreEqual(0, result.ExitCode); | ||
|
|
||
| // AssertValidPingOutput(result); |
There was a problem hiding this comment.
Nit: We want to remove unused/commented code prior to opening a PR.
| { | ||
| throw new AggregateException(ex.InnerException); | ||
| } | ||
| throw ex.Flatten().InnerException ?? ex; |
There was a problem hiding this comment.
In addition to Tyler's comment above, when you throw the InnerException here (or ex) we will actually lose the original stack trace on the exception. We want this information to potentially debug issues and understand where the exception was first thrown, even though this is a unit test.
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| return Run(hostNameOrAddress); | ||
| }, cancellationToken); | ||
| } | ||
|
|
||
| async public Task<PingResult> RunAsync(params string[] hostNameOrAddresses) |
There was a problem hiding this comment.
This modifier declaration order is inconsistent from the standard. It should be:
| async public Task<PingResult> RunAsync(params string[] hostNameOrAddresses) | |
| public async Task<PingResult> RunAsync(params string[] hostNameOrAddresses) | |
| await Task.WhenAll(all); | ||
| int total = all.Aggregate(0, (total, item) => total + item.Result); | ||
| return new PingResult(total, stringBuilder?.ToString()); | ||
| Task<PingResult>[] tasks = hostNameOrAddresses.Select((string x) => RunAsync(x)).ToArray(); |
There was a problem hiding this comment.
It looks like you are failing to capture all the ping output from the processes here.
LeoMarlow
force-pushed
the
Assignment9+10-Multithreading
branch
2 times, most recently
from
9686688 to
4c10012
Compare
SummarySummary
CoverageAssignment - 82.3%
|
My partner for this assignment was @LeoMarlow